-
-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
replace flake8-eradicate with ruff ... and enable a ton of more lint rules, fixing a couple of them. #196
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I've been vaguely interested in trying ruff
for a while and this looks like a clean replacement. IMO we could drop the flake8 checks immediately, if there's nothing else left out?
flake8_trio/visitors/visitor100.py
Outdated
from typing import Any | ||
from typing import TYPE_CHECKING, Any | ||
|
||
import libcst as cst |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aaaand, let's disable this as well - it's pretty verbose and doesn't actually have any performance benefits here at all; we're executing all the imports already when we import the matchers.
(perils of starting with such a long list of available lint rules!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, looks like flake8-type-checking has explicit logic about ignoring the case when you import a submodule. I'll add libcst
to https://beta.ruff.rs/docs/settings/#exempt-modules for now but might want to stick with flake8-type-checking
over ruff and/or open an issue in ruff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opened astral-sh/ruff#4667
I don't mind running both for a while to see where they agree/disagree. But if it becomes bothersome I'll quickly start disabling error codes/plugins, set the |
…rules, fixing a couple of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah shoot, I was just a few minutes off pushing another minor change to this x) I'll do it in a separate PR~ |
If enabling auto-updating of pre-commit, flake8-eradicate will come to bite us as it's not accepting PR's to show that it works with flake8 6. I was planning to add an updated fork as additional_dependency to flake8 instead, but I stumbled upon https://github.com/charliermarsh/ruff instead - which, among others, reimplements flake8-eradicate. Curiosity peaked, I added it to CI, enabled everything (it sucks there's no
--select-all
), and after playing around for a bit it looks to be almost strictly better than flake8 in a lot of ways. It implements a ton of plugins by default, has autofix for a bunch of them, and in general very easy to transition to from flake8.flake8-mutable is the only plugin flake8-trio uses I didn't see listed in https://github.com/charliermarsh/ruff#rules (though maybe there is a rule for default mutable arguments anyway, haven't checked), and flake8-noqa (but there is support for checking noqa stuff).
Looks like they haven't kept up reimplementing all rules from flake8-bugbear (esp not from the opinionated ones), but majority of them are in.
So might be interesting to enable it, and see how viable it is as a replacement for flake8.